Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple Chargeback from ManageIQ/manageiq-consumption since it's not… #19550

Closed
wants to merge 2 commits into from

Conversation

gtanzillo
Copy link
Member

We are planning on archiving the ManageIQ/manageiq-consumption repository because we are not currently using it and do not have future plans to use it. Before that can happen, references in Chargeback need to be removed.

@gtanzillo gtanzillo force-pushed the decouple-consumption branch from df7eb32 to 4610293 Compare November 22, 2019 21:16
@gtanzillo gtanzillo removed the wip label Nov 26, 2019
@gtanzillo gtanzillo changed the title [WIP] Decouple Chargeback from ManageIQ/manageiq-consumption since it's not… Decouple Chargeback from ManageIQ/manageiq-consumption since it's not… Nov 26, 2019
@chessbyte
Copy link
Member

@gtanzillo is this the removal of the code developed with @chargio ? if so, I would like to discuss that before it is removed. /cc @Fryguy

@gtanzillo
Copy link
Member Author

That's correct, this is the removal of the "Chargio" integration. I'll make this PR WIP so that we can discuss

@gtanzillo gtanzillo changed the title Decouple Chargeback from ManageIQ/manageiq-consumption since it's not… [WIP] Decouple Chargeback from ManageIQ/manageiq-consumption since it's not… Nov 27, 2019
@gtanzillo gtanzillo added the wip label Nov 27, 2019
@chargio
Copy link
Contributor

chargio commented Nov 28, 2019

It is a pity but I understand why you don't want to have code that you are not using.

I still believe that this code is faster and easier to maintain that the existing one, though.

@lpichler
Copy link
Contributor

lpichler commented Dec 4, 2019

we need to remove also those methods():

ChargeableField#showback_measure
ChargeableField#showback_dimension

Chargeback#showback_category
ChargebackRateDetail#showback_unit

but I think your reason was to just unplugging consumption repo(those methods became just dead code after merging this PR) - so I can do it in follow up 👍

otherwise looks good to me 👍 Thanks!

@Fryguy
Copy link
Member

Fryguy commented Dec 5, 2019

The code will still be in the repo so we can always bring it back. However, right now, that repo has failing tests with no one to fix them, and the maintenance burden is high for something we are not actually using.

@gtanzillo
Copy link
Member Author

@lpichler I'm assuming that we would also need a migration to remove all of the showback_* tables.

@gtanzillo gtanzillo force-pushed the decouple-consumption branch from 4610293 to 59fc065 Compare December 5, 2019 20:41
@miq-bot
Copy link
Member

miq-bot commented Dec 5, 2019

Checked commits gtanzillo/manageiq@6e3dc43~...59fc065 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🍪

@Fryguy
Copy link
Member

Fryguy commented Dec 17, 2019

This seems ready to go, so can it be un-WIP-ed?

I spoke to @chessbyte around this and we was good with it as long as it's just decoupling. Since manageiq-consumption repo will still exist, we can always bring this back in the future.

@gtanzillo gtanzillo changed the title [WIP] Decouple Chargeback from ManageIQ/manageiq-consumption since it's not… Decouple Chargeback from ManageIQ/manageiq-consumption since it's not… Dec 17, 2019
@gtanzillo gtanzillo added wip and removed wip labels Dec 17, 2019
@miq-bot
Copy link
Member

miq-bot commented Jun 11, 2020

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

@miq-bot miq-bot added the stale label Jun 11, 2020
@gtanzillo gtanzillo removed the stale label Jul 20, 2020
@miq-bot
Copy link
Member

miq-bot commented Feb 9, 2021

This pull request is not mergeable. Please rebase and repush.

@Fryguy
Copy link
Member

Fryguy commented Feb 11, 2021

@gtanzillo I didn't realize this code was at a mergeable state - can you rebase?

@chessbyte
Copy link
Member

@Fryguy are you sure we want to remove this? I thought that this was the strategic direction for chargeback.

@Fryguy
Copy link
Member

Fryguy commented Feb 11, 2021

@Fryguy See #19550 (comment) - While there are no failing tests at the moment, it is still a maintenance overhead.

@gtanzillo gtanzillo closed this Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants